Followup to #895: rename dissat_sat function and use struct rather than tuple for return values#930
Merged
Conversation
Member
Author
|
On 5097641 successfully ran local tests |
trevarj
approved these changes
Apr 23, 2026
Comment on lines
+486
to
+489
| let (dissats, sats): (Vec<_>, Vec<_>) = sat_dissats | ||
| .into_iter() | ||
| .map(|SatDissat { dissat, sat }| (dissat, sat)) | ||
| .unzip(); |
Contributor
There was a problem hiding this comment.
let (dissats, sats): (Vec<_>, Vec<_>) = stack
.drain(stack.len() - thresh.n()..)
.map(|SatDissat { dissat, sat }| (dissat, sat))
.unzip();
Member
Author
There was a problem hiding this comment.
Ah, yeah, I should combine these two let lines.
Member
Author
There was a problem hiding this comment.
I think in an earlier iteration of this PR I was still using the sat_dissat variable, but clearly I am not now..
Member
Author
There was a problem hiding this comment.
Heh, and I don't think my "allocation-wasteful" comment actually applies anymore. Nice.
| } | ||
| } | ||
|
|
||
| /// The (dissatisfaction, satisfaction) pair for an `older` fragment. |
Contributor
There was a problem hiding this comment.
minor nit, can ignore: Find and replace on "(dissatisfaction, satisfaction) pair" to just "satisfaction and dissatisfaction" or "SatDissat" in the doc comments.
apoelstra
force-pushed
the
2026-04/followup-895
branch
from
5097641 to
5870086
Compare
Member
Author
|
Fixed both nits. |
trevarj
reviewed
Apr 24, 2026
| // order of our tuple. | ||
| let (dissats, sats): (Vec<_>, Vec<_>) = stack | ||
| .drain(stack.len() - thresh.n()..) | ||
| .into_iter() |
Contributor
There was a problem hiding this comment.
Any reason for still using into_iter()? It compiled without it for me.
Contributor
There was a problem hiding this comment.
Ah clippy picked up on it too.
Member
Author
There was a problem hiding this comment.
Hah, nice, we're saving LOC all over the place. Fixed.
It is too easy to get the satisfactions and dissatisfactions confused. Use a struct with named fields. Curiously, outside of this module dissatisfactions are never used, so we can keep the `dissat` field private. (Well, this is a bit of a sketchy thing to say -- dissatisfactions **are** used by the Satisfaction::thresh and thresh_mall functions, but those functions take dissats and sats as independent Vecs they can manipulate, rather than taking a SatDissat struct.)
The name of the function no longer implies its return value (which is a struct rather than a tuple that needs to be carefully ordered). So change the name to be consistent with the name of the module.
apoelstra
force-pushed
the
2026-04/followup-895
branch
from
5870086 to
f7f1689
Compare
Member
Author
|
On f7f1689 successfully ran local tests |
tcharding
approved these changes
Apr 28, 2026
Member
|
I didn't do any grepping because it sounds like @trevarj already did. |
apoelstra
added a commit
that referenced
this pull request
May 20, 2026
…on` in the satisfier 6fd1cc3 miniscript: change tap_leaf_internal to return an option (Andrew Poelstra) 15c093f miniscript: use downcasting logic for computing a taproot leaf hash (Andrew Poelstra) f946d4a clippy: fix a couple new nightly lints (Andrew Poelstra) Pull request description: Now that we have #895 and #930 we have room to change the API for helpers to the satisfaction functions. Change the method `tap_leaf_internal`, which currently computes the "tapleaf hash" for an arbitrary Miniscript, even non-Taproot ones, to detect whether this is a Taproot script and only compute the tapleaf hash for Taproot scripts. Once we are returning an `Option` from `tap_leaf_internal`, we can change a bunch of satisfaction code that currently takes a `Ctx` paramater and introspects it to determine whether we are in Taproot mode or not, and have it simply use the `Option` to decide. Aside from speeding up and simplifying the code, this removes several instances of the `Ctx` parameter, which we want to eventually eliminate. ACKs for top commit: apoelstra: ACK 6fd1cc3; successfully ran local tests tcharding: ACK 6fd1cc3 Tree-SHA512: c4f6f41684f7adf3b0d7c7eb0cd4d3d40be8ffef872a8e918d430ddf26ae2b7e089e8534e7fea8eb64cbf0c51b58ad63637799356e5e4d4f071abf0cf7aa51d8
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Makes naming more consistent and reduces confusion caused by having a tuple with two entries of the same type.